Core: Store object name separately for symbols #13113

CelestialAmber · 2024-10-10T00:12:24Z

This PR adds code that checks for extra information following symbol names that is often present in CodeWarrior symbol maps, namely the names of the .a/.o files that symbol belongs to. If present, the symbol name string is truncated to remove the extra text from the symbol name.

JMC47 · 2024-10-10T02:42:50Z

@dolphin-emu-bot rebuild

CelestialAmber · 2024-10-10T04:54:00Z

I'm changing this to draft as I think it would be better to also include code to handle the file name instead of just discarding it. I would like to also update the symbol view accordingly to still show file names but that might be better as a separate PR so I'm open to discussions on that.

CelestialAmber · 2024-10-10T20:54:33Z

Ok, ready for review again.

sepalani · 2024-10-11T03:01:47Z

I suspect that this change can break other symbol map formats. Moreover, the object might not necessarily be a .o object file.

IIRC, the stripped symbol name is already available through the Symbol::function_name member. What I could suggest you instead is to provide a GUI toggle that can switch between displaying the stripped function name or the whole name including everything that's following it. It might be useful when working with relocated symbols where some symbols have the same name but different address and object.

CelestialAmber · 2024-10-11T03:34:18Z

Those are fair concerns; I do recall some map files having .c, .s, etc at the end. However, it would be very easy to remedy that issue - all that would need to be done is just to pick the last part of the split string, as it will always be the object file. I would be willing to test it against various map formats if so desired; I think that it would be a shame to just settle for a GUI toggle instead of being able to develop on top of this, perhaps adding the option to filter by object file (something I also want to try in the future if this is accepted). I feel that the debugging features in Dolphin have a lot of room for improvement and while I understand that this definitely needs to be made more robust, I would be willing to put in the necessary work.

sepalani

Your changes are going way beyond just adding the object name properties. You're changing the way symbol maps are loaded and saved.

I couldn't test it because it doesn't compile but I highly suspect that regular expressions might have a performance impact compared to the original code. The way you're matching for the object name might also be too strict.

IMO, I consider changing the name value not fine, as it forces you to change the logic at so many places. For instance, all those ternary could have been avoided.

In sum, I'd advise you to add an object_name member as a first PR. Then, build features on top of it with follow-up PRs.

Source/Core/Core/Boot/ElfReader.cpp

Source/Core/Core/Debugger/RSO.cpp

Source/Core/Core/PowerPC/PPCSymbolDB.cpp

Source/Core/Core/PowerPC/PPCSymbolDB.h

CelestialAmber · 2024-10-13T19:41:41Z

Given how large the PR has grown, I suppose it is reasonable that only the object name split changes are included in this PR, and the rest can be considered after.

That being said, I did timing tests, and this is what I got:

Original:
Debug: 0.885 s
Release: 0.146 s

Branch:
Debug: 33.097 s
Release: 1.0899 s

So, needless to say, there is definitely lots of room for improvement.

CelestialAmber · 2024-10-13T20:09:27Z

I decided to essentially revert the regex changes for this PR, and add a few additional checks to handle getting the actual symbol name and object name. The performance is only slightly worse than the current mainline branch:

Debug: 1.507 s
Release: 0.187 s

JMC47 · 2024-10-18T17:28:19Z

@dolphin-emu-bot rebuild

sepalani

I tested it with some symbol maps and it doesn't play well with them.

Could you please squash your commits into a single one and address the buildbot lint and build failures?

Source/Core/Core/PowerPC/PPCSymbolDB.cpp

sepalani · 2024-10-20T17:17:16Z

Source/Core/Core/PowerPC/PPCSymbolDB.cpp

+    std::string line = fmt::format("{0:08x} {1:08x} {2:08x} {3} {4}", symbol->address, symbol->size,
+                                   symbol->address, 0, symbol->name);
+    // Also write the object name if it exists
+    if (!symbol->object_name.empty())
+      line += fmt::format(" {0}", symbol->object_name);
+    line += "\n";
+    f.WriteString(line);
  }

  // Write .data section
  f.WriteString("\n.data section layout\n");
  for (const auto& symbol : data_symbols)
  {
    // Write symbol address, size, virtual address, alignment, name
-    f.WriteString(fmt::format("{0:08x} {1:08x} {2:08x} {3} {4}\n", symbol->address, symbol->size,
-                              symbol->address, 0, symbol->name));
+    std::string line = fmt::format("{0:08x} {1:08x} {2:08x} {3} {4}", symbol->address, symbol->size,
+                                   symbol->address, 0, symbol->name);
+    // Also write the object name if it exists
+    if (!symbol->object_name.empty())
+      line += fmt::format(" {0}", symbol->object_name);
+    line += "\n";
+    f.WriteString(line);


Some symbols have spaces when not mangled, so separating the symbol name and the object name with a space instead of a tab (like codewarrior symbol map) will most likely break Dolphin's parsing when loading these saved map.

Zelda TP:

00000000 000054 80006920 4 SECallback(int, int) d_home_button.o

This part isn't addressed. I highly recommend you to separate the symbol name and its object with a tab. Some symbols have multiple objects (e.g. AIInit<space><tab>ai.a<space>ai.o).

Source/Core/Core/PowerPC/PPCSymbolDB.cpp

mitaclaw · 2024-10-26T21:12:10Z

@dolphin-emu-bot rebuild

sepalani · 2024-11-01T13:52:36Z

Source/Core/Core/PowerPC/PPCSymbolDB.cpp

+    std::string line = fmt::format("{0:08x} {1:08x} {2:08x} {3} {4}", symbol->address, symbol->size,
+                                   symbol->address, 0, symbol->name);
+    // Also write the object name if it exists
+    if (!symbol->object_name.empty())
+      line += fmt::format(" {0}", symbol->object_name);
+    line += "\n";
+    f.WriteString(line);
  }

  // Write .data section
  f.WriteString("\n.data section layout\n");
  for (const auto& symbol : data_symbols)
  {
    // Write symbol address, size, virtual address, alignment, name
-    f.WriteString(fmt::format("{0:08x} {1:08x} {2:08x} {3} {4}\n", symbol->address, symbol->size,
-                              symbol->address, 0, symbol->name));
+    std::string line = fmt::format("{0:08x} {1:08x} {2:08x} {3} {4}", symbol->address, symbol->size,
+                                   symbol->address, 0, symbol->name);
+    // Also write the object name if it exists
+    if (!symbol->object_name.empty())
+      line += fmt::format(" {0}", symbol->object_name);
+    line += "\n";
+    f.WriteString(line);


This part isn't addressed. I highly recommend you to separate the symbol name and its object with a tab. Some symbols have multiple objects (e.g. AIInit<space><tab>ai.a<space>ai.o).

Source/Core/Core/Debugger/RSO.cpp

Source/Core/Core/PowerPC/PPCSymbolDB.cpp

Source/Core/Core/Boot/Boot.cpp

Source/Core/DolphinQt/Debugger/CodeWidget.cpp

Source/Core/Core/PowerPC/PPCSymbolDB.cpp

CelestialAmber · 2024-11-02T20:04:06Z

@dolphin-emu-bot rebuild

sepalani · 2024-11-02T20:14:47Z

Source/Core/Core/PowerPC/PPCSymbolDB.cpp

+    std::vector<std::string> parts = SplitString(name, '\t');
+    size_t num_parts = parts.size();


Can't they be marked as const?

Source/Core/Core/PowerPC/PPCSymbolDB.cpp

sepalani

Were those leading spaces change intentional? If so, please make such change appear in the commit message and/or in the GitHub conversation as they can be missed quite easily. If they weren't intentional, please make sure you double-check your change before pushing them.

Otherwise, you should also squash your commits into a single one.

Source/Core/Core/PowerPC/PPCSymbolDB.cpp

CelestialAmber · 2024-11-11T15:20:53Z

The leading spaces are intentional.

sepalani

This leading spaces change will likely break tools designed to load Dolphin generated symbol maps, especially if they don't take into account leading spaces.

There exist symbol maps generated without these leading spaces. However, I can't confirm that these were generated with CW.

Source/Core/Core/PowerPC/PPCSymbolDB.cpp

CelestialAmber · 2024-11-11T16:58:00Z

This leading spaces change will likely break tools designed to load Dolphin generated symbol maps, especially if they don't take into account leading spaces.

There exist symbol maps generated without these leading spaces. However, I can't confirm that these were generated with CW.

I understand there likely are some tools that might break from this, however there also are many tools that account for, or even assume the CodeWarrior symbol map format, which always includes leading spaces before symbols. For example, https://github.com/Cuyler36/Ghidra-GameCube-Loader/ does not support Dolphin symbol maps as is.

sepalani · 2024-11-11T17:12:22Z

This leading spaces change will likely break tools designed to load Dolphin generated symbol maps, especially if they don't take into account leading spaces.
There exist symbol maps generated without these leading spaces. However, I can't confirm that these were generated with CW.

I understand there likely are some tools that might break from this, however there also are many tools that account for, or even assume the CodeWarrior symbol map format, which always includes leading spaces before symbols. For example, https://github.com/Cuyler36/Ghidra-GameCube-Loader/ does not support Dolphin symbol maps as is.

IIRC, Dolphin already provides scripts to save/load symbol maps from/to IDA/Ghidra in Tools.

Your PR is changing how symbol maps are saved, which wasn't the initial goal of your PR, nor was it in your initial code.

I'm fine with adding the object info to the Symbol struct. However, if your plan is to load/save CW symbol maps in a strict manner, then you should provide the appropriate functions instead of changing the existing ones. For instance, you can add in the GUI (the load/save dialog or a dedicated menu) another symbol map format support.

CelestialAmber · 2024-11-11T17:25:43Z

Fine, I'll compromise. I would like to discuss things further though later.

CelestialAmber · 2024-11-20T17:53:39Z

@sepalani Any updates?

sepalani

The code LGTM, tested with some games and it seems to work properly.

I think that it'd be better to have at least someone else (with a more involved symbol map/debugger use) to test this PR to make sure that it doesn't break on their end. Maybe @TryTwo or @dreamsyntax?

dreamsyntax · 2024-11-23T21:27:46Z

I'll try with a few maps and see how it looks soon.

dreamsyntax · 2024-11-25T22:38:22Z

It would be nice to warn somehow that existing maps will be formatted, for those who do group collab/diffing - it could get messy if they don't know.

-800ab58c 00000030 800ab58c 0 HAS_Player::StartJumpCommand_AND_Player::TouchHangCommand(?Regular2PWorkingPulley?)_800ab58c_
+800ab58c 000030 800ab58c 0 HAS_Player::StartJumpCommand_AND_Player::TouchHangCommand(?Regular2PWorkingPulley?)_800ab58c_^M

[x] Old map format loads fine, saves in new format
[x] .data section preserved as expected for non-functions
[x] Total lines matching 1:1 - tested with GUP*.map and RSOs from RSVE8P
[x] Test a game with full built-in symbols (Spyro A Hero's Tail)

Edit: If you saw my message about an issue with Spyro, disregard. I made a mistake in my diffing.

dreamsyntax · 2024-11-25T22:42:46Z

Approved with the caveat that I cannot explicitly test the .a/.o detection/storage part of the PR as none of my games (that I know of) have this condition.

My other user made maps and built in maps work as expected.

TryTwo · 2024-11-26T01:54:22Z

I don't have anything to test. All my personal symbols were switched over to an extra note-layer in my personal debugger builds, because I like to mark certain instructions on top of symbol marking the functions.

Conforming to new format introduced in dolphin-emu/dolphin#13113

CelestialAmber marked this pull request as draft October 10, 2024 04:51

CelestialAmber changed the title ~~PPCSymbolDB: remove extra info from symbol names if present~~ [DRAFT] PPCSymbolDB: remove extra info from symbol names if present Oct 10, 2024

CelestialAmber marked this pull request as ready for review October 10, 2024 20:52

CelestialAmber changed the title ~~[DRAFT] PPCSymbolDB: remove extra info from symbol names if present~~ PPCSymbolDB: Store symbol object names from loaded symbol maps separately Oct 10, 2024

sepalani suggested changes Oct 13, 2024

View reviewed changes

CelestialAmber requested a review from sepalani October 14, 2024 14:45

CelestialAmber force-pushed the mwld-map branch from e701825 to 428032b Compare October 18, 2024 17:20

CelestialAmber changed the title ~~PPCSymbolDB: Store symbol object names from loaded symbol maps separately~~ Core: Store object name separately for symbols Oct 18, 2024

CelestialAmber force-pushed the mwld-map branch 2 times, most recently from 3e853bb to 568779d Compare October 18, 2024 17:24

sepalani suggested changes Oct 20, 2024

View reviewed changes

CelestialAmber force-pushed the mwld-map branch 2 times, most recently from 9a94563 to ce68a03 Compare October 24, 2024 19:27

CelestialAmber requested a review from sepalani October 24, 2024 19:28

sepalani suggested changes Nov 1, 2024

View reviewed changes

CelestialAmber force-pushed the mwld-map branch from b911754 to 225481a Compare November 2, 2024 02:28

sepalani suggested changes Nov 2, 2024

View reviewed changes

CelestialAmber force-pushed the mwld-map branch from 5ac2ac7 to 3ab07c9 Compare November 2, 2024 20:37

CelestialAmber closed this Nov 2, 2024

CelestialAmber force-pushed the mwld-map branch from 3ab07c9 to 97931a7 Compare November 2, 2024 20:46

CelestialAmber reopened this Nov 2, 2024

CelestialAmber force-pushed the mwld-map branch from 9e1910c to 63ffc4a Compare November 2, 2024 20:55

sepalani suggested changes Nov 2, 2024

View reviewed changes

Source/Core/Core/PowerPC/PPCSymbolDB.cpp Outdated Show resolved Hide resolved

sepalani suggested changes Nov 11, 2024

View reviewed changes

Source/Core/Core/PowerPC/PPCSymbolDB.cpp Outdated Show resolved Hide resolved

Source/Core/Core/PowerPC/PPCSymbolDB.cpp Outdated Show resolved Hide resolved

CelestialAmber force-pushed the mwld-map branch from c2bf177 to 55ac242 Compare November 11, 2024 16:13

sepalani suggested changes Nov 11, 2024

View reviewed changes

Source/Core/Core/PowerPC/PPCSymbolDB.cpp Outdated Show resolved Hide resolved

Source/Core/Core/PowerPC/PPCSymbolDB.cpp Outdated Show resolved Hide resolved

CelestialAmber force-pushed the mwld-map branch from 55ac242 to aa95805 Compare November 11, 2024 17:27

Core: Store object name separately for symbols

1c4bfc3

CelestialAmber force-pushed the mwld-map branch from aa95805 to 1c4bfc3 Compare November 11, 2024 17:36

sepalani approved these changes Nov 23, 2024

View reviewed changes

dreamsyntax approved these changes Nov 25, 2024

View reviewed changes

OatmealDome merged commit 57b1234 into dolphin-emu:master Dec 7, 2024
13 checks passed

dreamsyntax added a commit to ShadowTheHedgehogHacking/shadow-symbols-and-dmw that referenced this pull request Dec 8, 2024

Update symbol format (Dolphin 2412-23)

6e00fee

Conforming to new format introduced in dolphin-emu/dolphin#13113

CelestialAmber deleted the mwld-map branch December 8, 2024 01:43

sepalani mentioned this pull request Dec 9, 2024

PPCSymbolDB: Refactor SymbolMap Save/Load #13195

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Store object name separately for symbols #13113

Core: Store object name separately for symbols #13113

CelestialAmber commented Oct 10, 2024

JMC47 commented Oct 10, 2024

CelestialAmber commented Oct 10, 2024

CelestialAmber commented Oct 10, 2024

sepalani commented Oct 11, 2024

CelestialAmber commented Oct 11, 2024

sepalani left a comment

CelestialAmber commented Oct 13, 2024

CelestialAmber commented Oct 13, 2024

JMC47 commented Oct 18, 2024

sepalani left a comment

sepalani Oct 20, 2024

sepalani Nov 1, 2024

mitaclaw commented Oct 26, 2024

sepalani Nov 1, 2024

CelestialAmber commented Nov 2, 2024

sepalani Nov 2, 2024

sepalani left a comment •

edited

Loading

CelestialAmber commented Nov 11, 2024

sepalani left a comment

CelestialAmber commented Nov 11, 2024

sepalani commented Nov 11, 2024

CelestialAmber commented Nov 11, 2024

CelestialAmber commented Nov 20, 2024

sepalani left a comment •

edited

Loading

dreamsyntax commented Nov 23, 2024

dreamsyntax commented Nov 25, 2024 •

edited

Loading

dreamsyntax commented Nov 25, 2024

TryTwo commented Nov 26, 2024

		std::vector<std::string> parts = SplitString(name, '\t');
		size_t num_parts = parts.size();

Core: Store object name separately for symbols #13113

Core: Store object name separately for symbols #13113

Conversation

CelestialAmber commented Oct 10, 2024

JMC47 commented Oct 10, 2024

CelestialAmber commented Oct 10, 2024

CelestialAmber commented Oct 10, 2024

sepalani commented Oct 11, 2024

CelestialAmber commented Oct 11, 2024

sepalani left a comment

Choose a reason for hiding this comment

CelestialAmber commented Oct 13, 2024

CelestialAmber commented Oct 13, 2024

JMC47 commented Oct 18, 2024

sepalani left a comment

Choose a reason for hiding this comment

sepalani Oct 20, 2024

Choose a reason for hiding this comment

sepalani Nov 1, 2024

Choose a reason for hiding this comment

mitaclaw commented Oct 26, 2024

sepalani Nov 1, 2024

Choose a reason for hiding this comment

CelestialAmber commented Nov 2, 2024

sepalani Nov 2, 2024

Choose a reason for hiding this comment

sepalani left a comment • edited Loading

Choose a reason for hiding this comment

CelestialAmber commented Nov 11, 2024

sepalani left a comment

Choose a reason for hiding this comment

CelestialAmber commented Nov 11, 2024

sepalani commented Nov 11, 2024

CelestialAmber commented Nov 11, 2024

CelestialAmber commented Nov 20, 2024

sepalani left a comment • edited Loading

Choose a reason for hiding this comment

dreamsyntax commented Nov 23, 2024

dreamsyntax commented Nov 25, 2024 • edited Loading

dreamsyntax commented Nov 25, 2024

TryTwo commented Nov 26, 2024

sepalani left a comment •

edited

Loading

sepalani left a comment •

edited

Loading

dreamsyntax commented Nov 25, 2024 •

edited

Loading